Delegate conditional-decode 412 decision to prefix-based-pd-decider#1684
Delegate conditional-decode 412 decision to prefix-based-pd-decider#1684dmitripikus wants to merge 8 commits into
Conversation
Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
|
Please open an issue and add it to the PR, Behavior change in the 412 gate needs to be called out explicitly. This is the substantive change, and it is easy to miss:
We need to update the README file about the conditional decode behaviour. |
…f/else" Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
| // a real cache miss. | ||
| func primaryEndpointHasCachedPrefix(logger logr.Logger, result *fwksched.SchedulingResult) bool { | ||
| // primaryDecodeEndpoint returns the first endpoint chosen by the primary | ||
| // profile, or nil when the result is empty or malformed. False-return reasons |
There was a problem hiding this comment.
I think the comment is stale.
The function no longer inspects the cache, it only returns the endpoint. So, the "cache miss" framing is now inaccurate.
There was a problem hiding this comment.
Fixed, Thank you!
| Code: errcommon.PreconditionFailed, | ||
| Msg: "no decode worker has the requested KV cache", | ||
| decider := d.requestControlPlugins.ConditionalDecodeDecider() | ||
| switch { |
There was a problem hiding this comment.
if decider == nil {
logger.V(logutil.DEBUG).Info("conditional-decode: no decider configured, forwarding")
} else {
endpoint := primaryDecodeEndpoint(logger, result)
if endpoint == nil || decider.ShouldRejectConditionalDecode(ctx, reqCtx.SchedulingRequest, endpoint) {
...
}
...
}There was a problem hiding this comment.
I've resolved this when taking care of lint problem.
Thanks!
…hange Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
|
@roytman , addressing your fist set of comments:
|
| - Plugin not declared in the config → gate disabled (the EPP forwards conditional-decode requests unconditionally). | ||
| - Prefix-cache state unreadable on the chosen endpoint (e.g. no approximate-prefix producer wired up) → fail closed (412). | ||
|
|
||
| This behavior differs from earlier releases, where the gate was a hard-coded check inside the director that forwarded on any cache hit and rejected only on a complete cache miss. The threshold is now in tokens (not blocks), is operator-configurable, and is opt-in via the plugin. |
There was a problem hiding this comment.
We should avoid temporal language as instructed in AGENTS.md.
Would be interesting to understand if your agent "willingly" disobeyed that instruction or it just lost it in context.
There was a problem hiding this comment.
@vMaroon , this is an interesting point.
In this case the agent didn't work according to the instruction.
The agent states that "it didn't deliberately set the rule aside, but it also didn't consciously check it against the wording I was producing..."
We probably need to explicitly re-check/re-run against AGENTS.md
In any case, I re-wrote the comment not to contain temporal language
|
|
||
| **Conditional-decode 412 gate** | ||
|
|
||
| When this plugin is configured, it also drives the EPP's RFC 7240 `Prefer: if-available` (conditional-decode) gate. The coordinator uses that header to mark a speculative early-decode attempt: forward to a decode worker only if its KV cache already covers the prompt, otherwise return HTTP 412 Precondition Failed so the coordinator restarts the pipeline at encode/prefill/decode. |
There was a problem hiding this comment.
Wonder if this gate can be described independently, with coordinator use as a note.
There was a problem hiding this comment.
I re-wrote the comments to describe the gate independently and pull the coordinator behavior into a note. Thank you!
| // given the chosen decode endpoint, should the request be rejected with HTTP | ||
| // 412 (because the local KV cache does not cover enough of the prompt) or | ||
| // forwarded? At most one such plugin is consulted per director. | ||
| type ConditionalDecodeDecider interface { |
There was a problem hiding this comment.
Description should be self-contained, and descriptive of what the module does.
There was a problem hiding this comment.
I re-phrased the description.
Thank you!
Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
Signed-off-by: Dmitri Pikus <DPIKUS@il.ibm.com>
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Move the RFC 7240
Prefer: if-available412 gate out of the director and intothe
prefix-based-pd-deciderplugin. OnenonCachedTokensthreshold now drivesboth decisions:
disaggregated P/D and conditional-decode rejection.
The director had simplistic and non-configurable implementation of decode condition.
Behavior change: the 412 gate now rejects when the non-cached suffix is at or above
nonCachedTokens(rather than rejecting only on a complete cache miss), andnonCachedTokens: 0disables the gate entirely. Deployments wanting the 412 gate must include- type: prefix-based-pd-deciderwith a non-zerononCachedTokensin their EPP config; deployments using the canonicalpd-epp-config.yamlalready declare the plugin and pick up the gate automatically. Seedocs/disaggregation.mdfor details.Which issue(s) this PR fixes:
1686
Changes
requestcontrol.ConditionalDecodeDeciderinterface; auto-registeredthrough
Config.AddPlugins.*PrefixBasedPDDeciderimplements it viaShouldRejectConditionalDecode,sharing
computeNonCachedTokenswith the existingdisaggregatepath(different fail-mode semantics: 412 fail-closed, disaggregation fail-open).
Release note